Skip to content

Conversation

@Rushikesh0125
Copy link
Contributor

Implemented access control facet and library

  • exposed intuitive functions like assertOnlyRole, hasRole, and getAdminRole
  • exposed functions for role management with revoke, renounce, and grant ops
  • implemented a library with base internal methods
  • same/similar error, event, and method names to oz

@mudgen
Copy link
Contributor

mudgen commented Oct 19, 2025

Thank you, I will be reviewing this. We probably want to change "assert" to require because "assert" means something else in Solidity. @0xlynett please take a look at this too.

@0xlynett
Copy link
Contributor

0xlynett commented Oct 19, 2025

_assertOnlyRole seems unneeded, replace each instance with something like:

if (!s._roles[role].hasRole[account]) revert AccessControlUnauthorizedAccount(account, role);

There's a lot of getStorage() that could be AccessControlStorage storage s = getStorage(); and s instead

Also, the storage struct is called LibERC165Storage and the storage position is compose.erc165, mistake when copying a different file as a template?

Thanks!

(I just remembered that the Ownable library lacks a requireOwner function as well, rip. I'll fix that later)

@0xlynett
Copy link
Contributor

Also the RoleData struct feels like it could just be split into two mappings such as _roleAdmin and _hasRole

@Rushikesh0125
Copy link
Contributor Author

Oh yeah, I headed to a diff direction at the start.

Noted, will do these changes.

@Rushikesh0125
Copy link
Contributor Author

Rushikesh0125 commented Oct 20, 2025

gm @mudgen @0xlynett, I have refactored the implementation as recommended

  • renamed assert to require
  • replaced getStorage references with preloaded storage ref
  • removed repetitive usage of assertOwner with in-line check and removed assertOwner internal function from facet
  • renamed misnamed storage position
  • reorg of role data struct to two mappings

@Rushikesh0125
Copy link
Contributor Author

Hey @mudgen @0xlynett, bumping this up for review

@mudgen
Copy link
Contributor

mudgen commented Oct 24, 2025

@Rushikesh0125, thanks I am looking at it

Copy link
Contributor

@mudgen mudgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rushikesh0125 Looks good. Just some styling issues to match the rest of the code base.

@mudgen
Copy link
Contributor

mudgen commented Oct 24, 2025

@Rushikesh0125 How compatible is this with OpenZeppelin's version? I am unsure if compatibility matters much with this functionality, but maybe, do you have an idea? Are there any ERC standards that cover this functionality?

@Rushikesh0125
Copy link
Contributor Author

@mudgen I've followed the same approach as Oz's implementation, with the addition of the recommended requireRole function to replace the modifier. I don't believe compatibility is a major concern here, since there's no specific (ERC) standard for this functionality. The oz/access library itself is generally accepted as the standard way to implement access control.

@Rushikesh0125
Copy link
Contributor Author

@mudgen, I have made the required changes.

Copy link
Collaborator

@panditdhamdhere panditdhamdhere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work @Rushikesh0125

@github-actions
Copy link

Coverage Report

Coverage

Metric Coverage Details
Lines 0% 0/592 lines
Functions 0% 0/92 functions
Branches 0% 0/124 branches

Last updated: Sun, 26 Oct 2025 15:11:29 GMT for commit 25d5396

@mudgen mudgen closed this Oct 26, 2025
@mudgen mudgen reopened this Oct 26, 2025
@mudgen mudgen merged commit 9f3b258 into Perfect-Abstractions:main Oct 26, 2025
1 of 3 checks passed
@mudgen
Copy link
Contributor

mudgen commented Oct 26, 2025

Good job!

JackieXu pushed a commit to JackieXu/Compose that referenced this pull request Nov 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants